otelkit: Allow specifying a custom span type#4044
Conversation
|
The span kind is strictly defined by the HTTP semantic conventions. See: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.21.0/specification/trace/semantic_conventions/http.md#http-server-semantic-conventions If you want to change the semantic conventions then you should create an issue (and maybe PR) in https://github.com/open-telemetry/semantic-conventions (new repo for semantic conventions). I think this PR should be closed or at least moved to draft. |
|
go-kit supports transports other than HTTP (e.g. it comes with a AMQP transport) - https://github.com/open-telemetry/opentelemetry-specification/blob/4229197f45f0da0642bc7c0127e84c8aa1439489/specification/trace/semantic_conventions/messaging.md#span-kind mentions using the consumer/producer span kinds and |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4044 +/- ##
=====================================
Coverage 79.2% 79.2%
=====================================
Files 165 165
Lines 10318 10324 +6
=====================================
+ Hits 8173 8179 +6
Misses 2008 2008
Partials 137 137
|
jackwilsdon
force-pushed
the
otelkit-custom-span-type
branch
from
416a83f to
05fff7b
Compare
jackwilsdon
force-pushed
the
otelkit-custom-span-type
branch
from
05fff7b to
4eb938c
Compare
pellared
left a comment
pellared
left a comment
There was a problem hiding this comment.
Second review after getting more feedback.
|
|
||
| // WithSpanKind sets the span kind for spans created by this middleware. | ||
| // If this option is not provided, then trace.SpanKindServer is used. | ||
| func WithSpanKind(kind trace.SpanKind) Option { |
There was a problem hiding this comment.
I think that only two values of trace.SpanKind are valid:
trace.SpanKindServertrace.SpanKindConsumer
Can we change and add WithSpanKindServer() and WithSpanKindConsumer() options instead?
Credits to @Aneurysm9
There was a problem hiding this comment.
I've been using SpanKindInternal on my endpoints and instead putting server/consumer on the transport's spans, as it's up to the transport whether the endpoint is called as part of a server or consumer. Changing this to just those two options somewhat limits the flexibility here.
The kit/tracing/opentracing package offers TraceEndpoint which allows using any span type.
There was a problem hiding this comment.
I am not following. Why would you set to Internal?
as it's up to the transport whether the endpoint is called as part of a server or consumer
How would you modify the span kind? If it would be possible then you would not even need this functionality.
However, now I see that go-kit transport/endpoint can also be a "consumer" or "client" so probably it is better to keep as it is. Approving again 😉
There was a problem hiding this comment.
I looked what the instrumentation does and it does not do any context extraction or injection, so is kind of makes internal spans... Also I do not think it applies any OTel Semantic Conventions.
I start to feel that this module should be deprecated and moved to https://github.com/go-kit/kit/tree/master/tracing 😬
|
Closing as during SIG meeting we agreed that we are dropping support for Still, thank you for your contribution ❤️ |
2 participants
This is useful for when an endpoint may be wrapping a call to another service, or when an endpoint is used as a consumer (e.g. with https://github.com/alebabai/go-kit-kafka). The go-kit opentracing provider supports this via use of
WithTags.